fix(op-acceptor): add build dependencies and increase logs #352

serpixel · 2025-05-14T18:57:45Z

Description

Add git and other build dependencies to the op-acceptor docker image.
Increase the timeout for listing tests as it downloads dependencies and increase logs
Add extra logging for tests

Tests

Added a test to verify the paths checks logic.

Additional context

When running the tests sometimes we may want to get extra information on these. Right now we need to ssh into the container, but this PR adds a flag to dump the logs into the console.

Requires ethereum-optimism/optimism#16050 for the test log level.

Metadata

Fixes #[Link to Issue]

scharissis · 2025-05-19T05:59:16Z

Do you still think this is needed?

codecov-commenter · 2025-05-20T20:19:17Z

Codecov Report

Attention: Patch coverage is 76.25000% with 19 lines in your changes missing coverage. Please review.

Project coverage is 55.66%. Comparing base (47212b6) to head (ea905aa).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
op-acceptor/nat.go	0.00%	10 Missing ⚠️
op-acceptor/runner/runner.go	89.70%	6 Missing and 1 partial ⚠️
op-acceptor/config.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
+ Coverage   55.48%   55.66%   +0.17%     
==========================================
  Files          75       75              
  Lines        9591     9646      +55     
==========================================
+ Hits         5322     5369      +47     
- Misses       3913     3920       +7     
- Partials      356      357       +1

Flag	Coverage Δ
op-acceptor	`56.78% <76.25%> (+0.54%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
op-acceptor/flags/flags.go	`57.14% <ø> (ø)`
op-acceptor/config.go	`0.00% <0.00%> (ø)`
op-acceptor/runner/runner.go	`66.70% <89.70%> (+1.60%)`	⬆️
op-acceptor/nat.go	`37.33% <0.00%> (-0.15%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

op-acceptor/Dockerfile

scharissis · 2025-05-20T23:21:35Z

op-acceptor/Dockerfile

+# Set ownership of the /app directory to allow the application to write logs and other files at runtime
+RUN chown -R app:app /app/


Is this needed? The logs and files are working without this, I believe?

This is needed if running the docker image locally unless we change the directory we run it on (like in CI).

op-acceptor/runner/runner.go

scharissis · 2025-05-21T11:23:05Z

op-acceptor/runner/runner.go

+	// Check if the path is available locally
+	if isLocalPath(metadata.Package) {
+		fullPath := filepath.Join(r.workDir, metadata.Package)
+		if _, statErr := os.Stat(fullPath); os.IsNotExist(statErr) {
+			r.log.Error("Local package path does not exist, failing test", "validator", metadata.ID, "package", metadata.Package, "fullPath", fullPath)
+			return &types.TestResult{
+				Metadata: metadata,
+				Status:   types.TestStatusFail,
+				Error:    fmt.Errorf("local package path does not exist: %s", fullPath),
+			}, nil
+		}
+	}


I haven't been able to find any evidence that this is needed.
I believe Go can and will resolve local paths, even when referenced as their full package paths.

Are you sure it's needed? How?

Without this part if you have a test that uses a local path in 'package' and this path doesn't exist then the entire test suite will fail and it may not be obvious why.
We're using such paths in our test file so this would be relevant.

I'm not sure I follow, can you give me an example?

op-acceptor/runner/runner.go

scharissis · 2025-05-21T11:32:04Z

op-acceptor/runner/runner.go

+	// Check if the path is available locally
+	if isLocalPath(metadata.Package) {
+		fullPath := filepath.Join(r.workDir, metadata.Package)
+		if _, statErr := os.Stat(fullPath); os.IsNotExist(statErr) {
+			r.log.Error("Local package path does not exist, failing test", "validator", metadata.ID, "package", metadata.Package, "fullPath", fullPath)
+			return &types.TestResult{
+				Metadata: metadata,
+				Status:   types.TestStatusFail,
+				Error:    fmt.Errorf("local package path does not exist: %s", fullPath),
+			}, nil
+		}
+	}


I haven't been able to find any evidence that this is required. My understanding, and experience, suggest that it's not because Go finds local packages (even when referenced by their full package paths).

Are you sure it's needed?

I added a comment above relating this, the tldr is that without this entry if you have a test that uses a local path in 'package' and this path doesn't exist then the entire test suite will fail.
We're using such paths in our test file so this would be relevant.

op-acceptor/runner/runner_test.go

scharissis · 2025-05-21T11:36:47Z

op-acceptor/runner/runner.go

+	// Always set the LOG_LEVEL environment variable
+	runEnv := append([]string{fmt.Sprintf("LOG_LEVEL=%s", r.testLogLevel)}, os.Environ()...)


Is this supposed to be TEST_LOG_LEVEL?
Does it get honored?

I have added a test for this to ensure the env variable is set. We need ethereum-optimism/optimism#16050 too in order for the tests to be honoring the log level.

But is it used? I don't see where?

scharissis · 2025-05-23T02:15:26Z

op-acceptor/runner/logging_test.go

+	// Verify that logs are not captured when OutputTestLogs is disabled
+	assert.Equal(t, types.TestStatusPass, testResult2.Status)
+	assert.Contains(t, testResult2.Stdout, "This is a test output")


Shouldn't this be asserting that it does NOT contain this output?

Suggested change

// Verify that logs are not captured when OutputTestLogs is disabled

assert.Equal(t, types.TestStatusPass, testResult2.Status)

assert.Contains(t, testResult2.Stdout, "This is a test output")

// Verify that logs are not captured when OutputTestLogs is disabled

assert.Equal(t, types.TestStatusPass, testResult2.Status)

assert.NotContains(t, testResult2.Stdout, "This is a test output")

Actually the input is shown either way because we're not saving it to a file, that's what the test itself is testing. I think the other pre-existing tests cover the file logging part so I didn't add for that.

I'm afraid I don't understand. How is this block of code verifying that logs are NOT captured?

// Verify that logs are not captured when OutputTestLogs is disabled assert.Equal(t, types.TestStatusPass, testResult2.Status) assert.Contains(t, testResult2.Stdout, "This is a test output")

scharissis · 2025-05-23T02:16:42Z

op-acceptor/runner/logging_test.go

@@ -68,3 +70,133 @@ gates:
 	assert.Contains(t, failingTest.Stdout, "This is some stdout output that should be captured")
 	assert.Contains(t, failingTest.Stdout, "This is a second line of output")
 }
+
+// TestLogLevelEnvironment verifies that the TEST_LOG_LEVEL environment variable is correctly set and used
+func TestLogLevelEnvironment(t *testing.T) {


Let's also test the different levels behave differently?

The test runner should be parsing the env variable (in the optimism repo) so I'm not sure we need to check the setup here

scharissis · 2025-05-27T20:11:18Z

op-acceptor/Dockerfile

@@ -9,7 +9,9 @@ COPY op-acceptor/ .

 RUN just build

-FROM alpine:3.21
+FROM golang:1.23.5-alpine3.21


I don't think we need the runner image to be updated, do we?

scharissis · 2025-05-27T20:26:50Z

op-acceptor/runner/runner.go

@@ -563,10 +601,12 @@ func (r *runner) runSingleTest(ctx context.Context, metadata types.ValidatorMeta

 	// If we couldn't parse the output for some reason, create a minimal failing result
 	if parsedResult == nil {
+		r.log.Error("test exited with non-zero exit code", "exitCode", cmd.ProcessState.ExitCode())


This line is confusing me; it need not necessarily have exited non-zero here?

scharissis · 2025-05-27T20:29:17Z

op-acceptor/runner/logging_test.go

+	// Verify that logs are not captured when OutputTestLogs is disabled
+	assert.Equal(t, types.TestStatusPass, testResult2.Status)
+	assert.Contains(t, testResult2.Stdout, "This is a test output")


I'm afraid I don't understand. How is this block of code verifying that logs are NOT captured?

// Verify that logs are not captured when OutputTestLogs is disabled assert.Equal(t, types.TestStatusPass, testResult2.Status) assert.Contains(t, testResult2.Stdout, "This is a test output")

serpixel requested a review from a team as a code owner May 14, 2025 18:57

serpixel requested a review from bitwiseguy May 14, 2025 18:57

serpixel added 4 commits May 20, 2025 21:44

fix(op-acceptor): ensure package paths are valid, increase timeouts

5a3506f

fix(op-acceptor): add extra logs and increase timeout

d35c624

fix(op-acceptor): switch to go base image and add build tools

9083dfd

feat(op-acceptor): add extended logging

f588c27

serpixel force-pushed the fix/op-acceptor/ensure-package-paths-timeout branch from 1dbbcec to f588c27 Compare May 20, 2025 19:45

fix(op-acceptor): failing test

5695367

serpixel changed the title ~~fix(op-acceptor): ensure package paths are valid, increase timeouts~~ fix(op-acceptor): add build dependencies and increase logs May 20, 2025

fix(op-acceptor): parse output

1663761

serpixel requested a review from scharissis May 20, 2025 21:18

scharissis requested changes May 21, 2025

View reviewed changes

serpixel added 2 commits May 21, 2025 15:35

fix(op-acceptor): comments

ca75ba0

fix(op-acceptor): add extra tests

ea905aa

serpixel linked an issue May 21, 2025 that may be closed by this pull request

op-acceptor: Output logs to the console and enable extra logs for tests when running in CI #374

Closed

scharissis requested changes May 23, 2025

View reviewed changes

scharissis reviewed May 27, 2025

View reviewed changes

		# Set ownership of the /app directory to allow the application to write logs and other files at runtime
		RUN chown -R app:app /app/

		// Always set the LOG_LEVEL environment variable
		runEnv := append([]string{fmt.Sprintf("LOG_LEVEL=%s", r.testLogLevel)}, os.Environ()...)

fix(op-acceptor): add build dependencies and increase logs #352

Are you sure you want to change the base?

fix(op-acceptor): add build dependencies and increase logs #352

Conversation

serpixel commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scharissis commented May 19, 2025

Uh oh!

codecov-commenter commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serpixel May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serpixel May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serpixel May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serpixel commented May 14, 2025 •

edited

Loading

codecov-commenter commented May 20, 2025 •

edited

Loading

serpixel May 21, 2025 •

edited

Loading

serpixel May 21, 2025 •

edited

Loading

serpixel May 27, 2025 •

edited

Loading